Skip to content

Conversation

alexey-ivanov-es
Copy link
Collaborator

@alexey-ivanov-es alexey-ivanov-es commented Mar 31, 2025

This PR addresses the following situation: we have a core component that injects a list of other components:

public class MyService {
    public MyService(List<A> listOfA) {...}
}

Where A is extensible:

@Extensible
class A {...}

These A components don't add any additional logic but simply provide configuration.

So instead of requiring them to extend A, the preferred approach is to create instances of A that can be registered with the injector:

@Extension
public static final A INSTANCE_1 = new A(...)

Let's look at this with a concrete example:
ThreadPool is service, which injects a collection of FixedExecutorBuilderSpec (only fixed for now) (in the future they should replace ExecutorBuilder)

FixedExecutorBuilderSpec provides all necessary information to create executor that will be used in ThreadPool.

FixedExecutorBuilderSpec is marked with annotation @Extensible

In Downsampling plugin we create an instance of FixedExecutorBuilderSpec and mark it with annotation @Extension:

    @Extension
    public static final FixedExecutorBuilderSpec DOWNSAMPLE_EXECUTOR = new FixedExecutorBuilderSpec(
        DOWNSAMPLE_TASK_THREAD_POOL_NAME,
        settings -> ThreadPool.oneEighthAllocatedProcessors(EsExecutors.allocatedProcessors(settings)),
        DOWNSAMPLE_TASK_THREAD_POOL_QUEUE_SIZE,
        "xpack.downsample.thread_pool"
    );

Copy link
Owner

@rjernst rjernst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine, couple nits


@Retention(RetentionPolicy.RUNTIME)
@Target(value = { TYPE, FIELD })
public @interface Extension {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wdyt of calling this "Constant"? Extension is very close to the existing Extensible...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe @instance or @InjectableInstance?

return List.of(downsample);
}
@Extension
public static final FixedExecutorBuilderSpec DOWNSAMPLE_EXECUTOR = new FixedExecutorBuilderSpec(
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we want this where it is used so the Plugin subclass can be removed. I believe that is in the downsample persistent task executor?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I left it here for now just to make the change easier to understand, but I can move it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants